Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display per-job metrics on Job Detail page #4024

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

iaindillingham
Copy link
Member

@iaindillingham iaindillingham commented Jan 24, 2024

This adds a "Job metrics" card to the Job Detail page for users with one or more roles. This card displays CPU and memory usage statistics, when available (see below).

In addition to mean and peak, which are displayed here, Job Runner collects sample and cumsum for both CPU and memory usage statistics (opensafely-core/job-runner#686). However, I don't think sample and cumsum usage statistics are useful to users: sample, because we (and so they) don't know when the sample was taken; cumsum, because we (and so they) don't know how many samples were taken (and if we did, then all we would be able to compute would be the mean, which Job Runner has computed for us).

As #3998 states, usage statistics are not available for historic jobs (i.e. prior to 17/01/2024). In this case, job.metrics == None. They are also not available for some other types of job. In these cases, job.metrics == {}. We don't distinguish these cases, displaying "-" to users in both cases.

Closes #3998


Thanks, @ghickman and @lucyb for suggesting different approaches to determine whether the "Job metrics" card should be displayed. I've opted for request.user.all_roles for simplicity. We do something similar, here:

{% if not user.all_roles %}
{% pill variant="info" text="User has no roles assigned" class="self-start" %}
{% endif %}

@sebbacon
Copy link
Contributor

It's not clear to me that this shouldn't be public information.

@iaindillingham asked:

Could you help me understand when information should be public and when it shouldn’t,
@seb?

  1. You need an account to use it
  2. Honeycomb is like an "expert" view. We'd have to explain lots of things for anyone to make use of it. Even if it were free, we're not in a position to make it useful for most users

In contrast, memory and CPU are fairly simple concepts. I don't think they add any extra noise, and I would personally go with the principle that most things should happen in public. You never know what benefits may accrue from exposing the data.

That said, I'm sure @lucyb had a good reason to suggest it should be for logged in users only in #3998...

@lucyb
Copy link
Contributor

lucyb commented Jan 25, 2024

That said, I'm sure @lucyb had a good reason to suggest it should be for logged in users only in

I was taking a precautionary approach. I wasn't sure the data should be public, since we're receiving it straight from the secure environments, and so I thought it would be best if it was for logged in users only. This also reduces the pressure to present it in a way that's meaningful to most people.

That said, I can't think of a reason why showing it to more people would actually cause an issue.

@iaindillingham
Copy link
Member Author

Thanks, @lucyb. If I've understood opensafely-core/job-runner#686 correctly, then I don't think there's a significant risk of leaking sensitive information. I also don't think that logged in users would be better at interpreting the metrics than public users; if there's an issue with their interpretation, in other words, then I think the issue would apply to both groups of users.

For these reasons, I'm going to remove the check. Sound good, @sebbacon?

@sebbacon
Copy link
Contributor

You know - now I've thought about this more, I think we should continue this conversation in a more brainstormy forum, and just :shipit: without it being public, for the time being.

Sorry for the distraction, but it's been useful!

@iaindillingham
Copy link
Member Author

Before I :shipit:, @ghickman asked on Slack about request.user.is_authenticated.

This information should be displayed on the Job Detail page to logged in users only.

I took "logged in users" from #3998 at face value: an authenticated user is also an authorized user. However, I think George was going to suggest that an authenticated user may not be an authorized user. Is that correct, George?

@ghickman
Copy link
Contributor

I think George was going to suggest that an authenticated user may not be an authorized user.

Yep, that's correct, user.is_authenticated does not imply permissions. This is important for us because we have open registration to support applications, so authenticated is barely a step above unauthenticated (by design) for us.

A better gate here would be to check for a permission in the view and pass that as a boolean out to the template via the context. The run_job permission seems like it would fit well here I think?

@lucyb
Copy link
Contributor

lucyb commented Jan 25, 2024

Alternatively, if you didn't want to be constrained by a particular permission, there's the all_roles property which you could do a truthy check on. It's a bit clunky but it's a pattern we've used in at least one other area.

This adds a "Job metrics" card to the Job Detail page for users with one
or more roles. This card displays CPU and memory usage statistics, when
available (see below).

In addition to mean and peak, which are displayed here, Job Runner
collects sample and cumsum for both CPU and memory usage statistics
(opensafely-core/job-runner#686). However, I don't think sample and
cumsum usage statistics are useful to users: sample, because we (and so
they) don't know when the sample was taken; cumsum, because we (and so
they) don't know how many samples were taken (and if we did, then all we
would be able to compute would be the mean, which Job Runner has
computed for us).

As #3998 states, usage statistics are not available for historic jobs
(i.e. prior to 17/01/2024). In this case, `job.metrics == None`. They
are also not available for some other types of job. In these cases,
`job.metrics == {}`. We don't distinguish these cases, displaying "-" to
users in both cases.

Closes #3998
@iaindillingham iaindillingham force-pushed the iaindillingham/per-job-metrics branch from 624b6b9 to 20e8e67 Compare January 26, 2024 15:31
@iaindillingham iaindillingham merged commit 8705608 into main Jan 26, 2024
6 checks passed
@iaindillingham iaindillingham deleted the iaindillingham/per-job-metrics branch January 26, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display per-job metrics on Job Detail page
4 participants